Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add injectivity annotations #9500

Merged
merged 16 commits into from Jun 15, 2020
Merged

Conversation

garrigue
Copy link
Contributor

Add injectivity annotations on abstract types, to allow using them in GADTs or constrained parameters.

The syntax is as suggested:

type !'a

When combined with a variance, the variance comes first:

type +!'a t

See examples in testsuite/tests/typing-misc/injectivity.ml

@gasche
Copy link
Member

gasche commented Apr 27, 2020

Note: when we discussed this during a meeting, I suggested that you and @lpw25 write a short document that would explain, with examples, the current need for (and uses of) the feature (including the examples from Oleg, and the "injectivity witnesses" that @lpw25 mentioned). I think this would be very useful to follow what's going on -- I understand the general feature and I think agree with @lpw25 it is simple and nice to expose it, but I haven't been able to follow the recent practical interest in it.

@yallop
Copy link
Member

yallop commented Apr 27, 2020

I think this means that #5985 can finally be closed.

@yallop
Copy link
Member

yallop commented Apr 27, 2020

What's the situation with GADTs? I haven't looked at the implementation, but gave the branch a quick try and was surprised to discover that injectivity annotations for some indexes are rejected:

# type (!_, !_) eql = Refl : ('a, 'a) eql;;
Line 1, characters 0-39:
1 | type (!_, !_) eql = Refl : ('a, 'a) eql;;
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: In this definition, expected parameter variances are not satisfied.
       The 1st type parameter was expected to be injective invariant,
       but it is unrestricted.

@garrigue
Copy link
Contributor Author

@yallop There is no point in giving injectivity annotations for "nominal" (i.e. records and sums, including GADTs and extensible ones) types: their parameters are always injective by definition.

@yallop
Copy link
Member

yallop commented Apr 29, 2020

Understood. But since the annotations are correct, even if pointless, I think it's best to accept them when they're given.

@yallop
Copy link
Member

yallop commented Apr 29, 2020

In particular, I think this behaviour, where a variant definition matches an injective signature, but can't be marked as injective directly, is surprising:

# module M : sig type !_ t end = struct type _ t = X end;;
module M : sig type !_ t end

# module M : sig type !_ t end = struct type !_ t = X end;;
Line 1, characters 38-51:
1 | module M : sig type !_ t end = struct type !_ t = X end;;
                                          ^^^^^^^^^^^^^
Error: In this definition, expected parameter variances are not satisfied.
       The 1st type parameter was expected to be injective invariant,
       but it is unrestricted.

@garrigue
Copy link
Contributor Author

@yallop The last commit ignores injectivity annotations for nominal types, so this fixes your problem.

@lpw25
Copy link
Contributor

lpw25 commented Apr 29, 2020

You can see the injectivity witnesses people use here:

https://ocaml.janestreet.com/ocaml-core/latest/doc/base/Base/Type_equal/module-type-Injective/index.html

I'm not sure whether any uses of it are in our open source code, but I think how you use it is fairly obvious. I'll try and dig up an example of why you might use it when I have some time.

@garrigue
Copy link
Contributor Author

garrigue commented Apr 29, 2020

This PR is now ready for reviewing.

It implements injectivity annotations on type parameters, using the suggested syntax !. Injectivity comes after variance, so one can write type +!'a t.

The main motivation is solving the famous #5985, explained in my OCaml 2013 paper. In particular, this allows to define GADTs where parametric abstract types appear as index. See the nominal types RFC or Oleg's recent mail on the caml-list for examples of this problem.

Injectivity annotations do not make sense for nominal types (records or sum types), since all their parameters are injective by definition. For them, annotations are silently ignored.

For type abbreviations (public or private), the injectivity of parameters can be inferred from the body of the type, and annotations are only checked.

For abstract types and private row types, injectivity annotations should be satisfied by the actual implementation. They are kept in the type definition, and checked through module subtyping. Note that for private row types, injectivity can also be inferred in some cases.

For concrete examples, see the file testsuite/tests/typing-misc/injectivity.ml.

The implementation has two parts.

  • The first one, about injectivity of normal type parameters is only about adding syntax to express injectivity, and mapping it to the already existing injectivity bit of variances. There was almost nothing to change internally, since injectivity is already inferred. The main points are parsing (we allow both + ! and +!, without introducing a specific lexeme for +!), and printing (i.e. print injectivity annotations, but only where they are useful).
  • The second part concerns the injectivity of constrained type parameters in type abbreviations. In order to integrate them smoothly, their injectivity is now inferred (this was not the case before, except when the parameter itself appeared in the type), i.e. such a parameter is deemed injective if it appears at an injective position in the body of the type, or if all its type variables are injective. As a result, one never needs to annotate explicitly a type abbreviation, so that injectivity annotations are only shown when printing abstract types and private row types.

@garrigue
Copy link
Contributor Author

@lpw25 Thank you, this is clear enough indeed.
I suppose everybody would be happy not to have to do this kind of gymnastic.
So adding injectivity annotations seems to be a reasonable first step, all the more as the compiler already tracks it.

@garrigue
Copy link
Contributor Author

This PR is a bit strange, as most of the effort seems to be going into constrained type parameters in type abbreviations, which is a very minor feature.
I have added a bit more code, to get maximal expressiveness: it is possible to write types in a way such that one can infer all the possible injectivity, even with constrained type parameters. On the other hand, writing it in a different way may lose injectivity, but does it really matter when those types a rare enough.

@garrigue garrigue requested a review from yallop April 30, 2020 12:18
Copy link
Member

@yallop yallop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very welcome change!

I believe the code to be correct, but please see the comments attached to this review for some suggestions for small improvements to the interface, implementation and documentation.

typing/printtyp.ml Show resolved Hide resolved
parsing/parser.mly Show resolved Hide resolved
typing/outcometree.mli Outdated Show resolved Hide resolved
typing/typedecl_variance.ml Show resolved Hide resolved
manual/manual/refman/typedecl.etex Show resolved Hide resolved
Changes Outdated Show resolved Hide resolved
manual/manual/refman/typedecl.etex Outdated Show resolved Hide resolved
@gasche
Copy link
Member

gasche commented Jun 10, 2020

@yallop I wonder if you are satisfied with @garrigue's handling of your points, so that the PR can move further. (This said, it needs a full code review to move further, and I don't know who is volunteering for that, I'm currently not.)

Copy link
Contributor

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks correct to me and I'm in favour of the feature.

@@ -200,6 +208,18 @@ constraints, the variance properties of the type constructor
are inferred from its definition, and the variance annotations are
only checked for conformance with the definition.

Injectivity annotations only make sense for abstract types and private
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Injectivity annotations only make sense for abstract types and private
Injectivity annotations are only necessary for abstract types and private

@yallop
Copy link
Member

yallop commented Jun 12, 2020

@gasche: I'm happy with @garrigue's response and changes, so I've updated my review to "approve".

@gasche
Copy link
Member

gasche commented Jun 12, 2020

@garrigue this can be merged, but you need to fix conflicts first -- in the Changes, and in the generated parser.

@garrigue garrigue merged commit 603506a into ocaml:trunk Jun 15, 2020
abbysmal pushed a commit to ocaml-multicore/tezos that referenced this pull request Jun 9, 2021
See ocaml/ocaml#9500 for the introduction of
the breaking change within OCaml.
abbysmal pushed a commit to ocaml-multicore/tezos that referenced this pull request Jun 22, 2021
See ocaml/ocaml#9500 for the introduction of
the breaking change within OCaml.
zshipko pushed a commit to zshipko/tezos that referenced this pull request Oct 7, 2021
See ocaml/ocaml#9500 for the introduction of
the breaking change within OCaml.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants